-
-
Notifications
You must be signed in to change notification settings - Fork 163
Request meta tests boilerplate #1746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Request meta tests boilerplate #1746
Conversation
|
Thanks, this looks great! I've changed the base of this PR, so that status checks run. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1746 +/- ##
=======================================
Coverage 92.24% 92.25%
=======================================
Files 437 437
Lines 14860 14860
Branches 2451 2451
=======================================
+ Hits 13708 13709 +1
+ Misses 709 708 -1
Partials 443 443 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mrmarcosmagalhaes, do you still want to complete the work on this PR? If not, please let me know, so I can pick up from here. |
|
Good morning
I want to finalize this pull request.
Sorry for the late reply.
I had another child and i didn't have so much time. Until to next weekend i
will implement your suggestions 👌
Regards,
Marcos Magalhães
A quarta, 3/12/2025, 01:54, Bart Koelman ***@***.***>
escreveu:
… *bkoelman* left a comment (json-api-dotnet/JsonApiDotNetCore#1746)
<#1746 (comment)>
@mrmarcosmagalhaes <https://github.com/mrmarcosmagalhaes>, do you still
want to complete the work on this PR? If not, please let me know, so I can
pick up from here.
—
Reply to this email directly, view it on GitHub
<#1746 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEOSMSAGDVHA64QHCUHPVWL37Y7FXAVCNFSM6AAAAACBUJV36GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMMBUGY4TKNJTHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Congratulations! Looking forward to your contribution. |
|
@bkoelman Completed and consolidated RequestMetaTests. |
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
…rmarcosmagalhaes/JsonApiDotNetCore into request-meta-tests-boilerplate
|
@mrmarcosmagalhaes Can you take a look at the build errors? |
|
Yes, tomorrow i will check.
A segunda, 12/01/2026, 22:49, Bart Koelman ***@***.***>
escreveu:
… *bkoelman* left a comment (json-api-dotnet/JsonApiDotNetCore#1746)
<#1746 (comment)>
@mrmarcosmagalhaes <https://github.com/mrmarcosmagalhaes> Can you take a
look at the build errors?
—
Reply to this email directly, view it on GitHub
<#1746 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEOSMSFX5E7223H5O3YDGUT4GQQF3AVCNFSM6AAAAACBUJV36GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTONBQHA3DGNBTHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…rmarcosmagalhaes/JsonApiDotNetCore into request-meta-tests-boilerplate
This reverts commit 5ee4d79.
bkoelman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing all the feedback. Minus the comments posted here, I think RequestMetaTests is good to go. I've added a comment requesting to sync up the operations tests, which I'll take a closer look at next time.
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/OperationsRequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/MetaFakers.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/RequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/OperationsRequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/OperationsRequestMetaTests.cs
Outdated
Show resolved
Hide resolved
test/JsonApiDotNetCoreTests/IntegrationTests/Meta/OperationsRequestMetaTests.cs
Outdated
Show resolved
Hide resolved
bkoelman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some more feedback. The tests for operations are not quite in sync yet.
| } | ||
|
|
||
| [Fact] | ||
| public async Task Accepts_meta_in_remove_from_ToOne_relationship_request() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public async Task Accepts_meta_in_remove_from_ToOne_relationship_request() | |
| public async Task Accepts_meta_in_remove_from_ToMany_relationship_request() |
I don't understand why this was changed from to-many to to-one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name does not match the implementation. I’ve already reverted the test name and refactor de code.
| await _testContext.RunOnDatabaseAsync(async dbContext => | ||
| { | ||
| dbContext.ProductFamilies.Add(existingFamily); | ||
| existingTicket.ProductFamily = existingFamily; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to before the RunOnDatabaseAsync block, like this:
ProductFamily existingFamily = _fakers.ProductFamily.GenerateOne();
SupportTicket existingTicket = _fakers.SupportTicket.GenerateOne();
existingTicket.ProductFamily = existingFamily;|
|
||
| store.Document.Meta.Should().BeEquivalentToJson(documentMeta); | ||
|
|
||
| store.Document.Data.Should().NotBeNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document.Data is a struct, so it can never be null. Therefore, this line is redundant.
|
|
||
| store.Document.Meta.Should().BeEquivalentToJson(documentMeta); | ||
|
|
||
| store.Document.Data.Should().NotBeNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document.Data is a struct, so it can never be null. Therefore, this line is redundant.
|
|
||
| store.Document.Should().NotBeNull(); | ||
|
|
||
| store.Document.Data.SingleValue.Should().BeNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This endpoint updates a to-many relationship (data is an array). I don't think it makes sense to assert that data is not an object. So this assertion can be removed.
| } | ||
|
|
||
| [Fact] | ||
| public async Task Accepts_meta_in_add_ToMany_relationship_request() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public async Task Accepts_meta_in_add_ToMany_relationship_request() | |
| public async Task Accepts_meta_in_add_to_ToMany_relationship_request() |
| } | ||
|
|
||
| [Fact] | ||
| public async Task Accepts_meta_in_add_resource_request_with_ToMany_relationship_operation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming pattern is not simply to add "_operation" to all existing names.
This is an operation that updates a resource, along with its to-many relationship. Therefore, the name should be: "Accepts_meta_in_add_resource_operation_with_ToMany_relationship". Please verify the other names within OperationsRequestMetaTests.cs for correctness.
| SupportTicket existingTicket1 = _fakers.SupportTicket.GenerateOne(); | ||
| SupportTicket existingTicket2 = _fakers.SupportTicket.GenerateOne(); | ||
| ProductFamily existingFamily = _fakers.ProductFamily.GenerateOne(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existingFamily should come first (as it does in the operations equivalent), because that's where the scenario starts (see the request URL).
Applies to multiple tests.
| } | ||
|
|
||
| [Fact] | ||
| public async Task Accepts_meta_in_remove_from_ToOne_relationship_operation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is about to-many.
| } | ||
|
|
||
| [Fact] | ||
| public async Task Accepts_meta_in_atomic_remove_resource_operation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public async Task Accepts_meta_in_atomic_remove_resource_operation() | |
| public async Task Accepts_meta_in_remove_resource_operation() |
Closes #1500
QUALITY CHECKLIST